-
Notifications
You must be signed in to change notification settings - Fork 225
Make sure HeaderFooterPage can contents be scrolled #4704
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is sadly a side-effect of removing the weight(1f)
modifier. I'm not sure we can really do something about it 🫤 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think I found a solution: 33db77f#diff-2214751455c3308dfeb4d6dbe967ae5156c950c3c219e10251a8b95df840080c
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4704 +/- ##
========================================
Coverage 80.11% 80.11%
========================================
Files 2126 2126
Lines 56303 56317 +14
Branches 7021 7022 +1
========================================
+ Hits 45107 45120 +13
Misses 8780 8780
- Partials 2416 2417 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes, 2 remarks/questions
@Composable | ||
private fun Content( | ||
state: SecureBackupEnterRecoveryKeyState, | ||
) { | ||
val bringIntoViewRequester = remember { BringIntoViewRequester() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FocusRequester
is not enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That made the textfield visible, but only half of it (the other half was under the IME).
) { | ||
// Header | ||
header() | ||
Box(modifier = Modifier.weight(1f)) { | ||
Box { | ||
content() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add some padding between the footer and the content? This looks a bit weird without
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... maybe, but maybe that should be added for the content on each screen? Otherwise we need to remember we have 10-16.dp
here and substract it from whatever we have in the designs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ganfra just to remind you of this question: should we add a default padding for all screens and then diff with the designs or leave it at 0 and just apply the padding to each content component so it matches the designs 1:1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it be a padding on top of the button?
|
Content
.weight(1f)
modifier in theHeaderFooterView
content wrapper so it can be scrolled.SecureBackupEnterRecoveryKeyView
component when it's focused and the IME opens.TopAppBar
container color inFlowStepPage
transparent so the scrolling content is not cut-off by it.Motivation and context
The recovery key screen (and others) was not scrollable anymore, ruining the UX.
Screenshots / GIFs
There are a few screenshots in the PR.
Tests
Tested devices
Checklist